Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

Warnings cleanup #383

Merged
merged 7 commits into from
Apr 6, 2024
Merged

Warnings cleanup #383

merged 7 commits into from
Apr 6, 2024

Conversation

slava77
Copy link
Contributor

@slava77 slava77 commented Apr 3, 2024

This attempts to address compilation warnings in SDL/

  • shadowing
  • alpaka::memset 3rd argument is a byte (uint8)
  • maybe-initialized
  • a few consts are added in function arguments (mostly incidental; not systematic)

I think there is a bug in runPixelQuintupletDefaultAlgo where the cut on passPT5RZChiSquaredCuts looks like it needs to be applied only for pLS pt < 5 GeV, but it's apparently applied to all because pLS pt at that point of the code is not defined.

@slava77
Copy link
Contributor Author

slava77 commented Apr 3, 2024

/run standalone
/run cmssw

Copy link

github-actions bot commented Apr 3, 2024

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Copy link

github-actions bot commented Apr 3, 2024

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

@GNiendorf GNiendorf linked an issue Apr 3, 2024 that may be closed by this pull request
@@ -2577,7 +2575,7 @@ namespace SDL {

rzChiSquared = computePT5RZChiSquared(acc, modulesInGPU, lowerModuleIndices, rtPix, zPix, rts, zs);

if (pixelRadius < 5.0f * kR1GeVf) {
if (/*pixelRadius*/ 0 < 5.0f * kR1GeVf) { // FIXME: pixelRadius is not defined yet
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YonsiG this is the part I was talking about during the meeting

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether this was contributing to #265. I'll check this, as well as fix the other non-deterministic kernels that @YonsiG found.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for finding it Slava! I'm just curious, why do we want to only use the rzChiSquared only for < 5GeV? For larger pt, in principal, the track on rz should be very straight line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, in rz computations hopefully do not explode numerically and we could safely apply the cut everywhere (well, we do already, by virtue of the bug here).

Note though that computePT5RZChiSquared is not using the helix correction.
Why? Doesn't this lead to inefficiency at low pt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Slava, thank you for your explanation! I don't remember there is a reason to keep the linear constraint for pT5 r-z. I think it's just we haven't applied the helix here yet. Can add it on my to-do list if necessary.

@slava77 slava77 marked this pull request as ready for review April 3, 2024 23:31
@slava77
Copy link
Contributor Author

slava77 commented Apr 4, 2024

I think that this is ready for review; I guess we can pick a reviewer in the meeting tomorrow, unless someone volunteers sooner

SDL/Hit.h Show resolved Hide resolved
SDL/Module.h Show resolved Hide resolved
@ariostas
Copy link
Member

ariostas commented Apr 5, 2024

When compiling for cpu I get one last warning. (It's pretty easy to fix)

In file included from Kernels.h:9,
                 from Event.h:9,
                 from Event.cc:1:
In function 'void SDL::addQuintupletToMemory(triplets&, quintuplets&, unsigned int, unsigned int, uint16_t&, uint16_t&, uint16_t&, uint16_t&, uint16_t&, float&, float&, float&, float&, float&, float&, float&, float&, float&, float, float, float, float, uint8_t, unsigned int, bool)',
    inlined from 'void SDL::createQuintupletsInGPUv2::operator()(const TAcc&, SDL::modules, SDL::miniDoublets, SDL::segments, SDL::triplets, SDL::quintuplets, SDL::objectRanges, uint16_t) const [with TAcc = alpaka::AccCpuSerial<std::integral_constant<long unsigned int, 3>, long unsigned int>]' at Quintuplet.h:3104:40:
Quintuplet.h:206:52: warning: 'rzChiSquared' may be used uninitialized [-Wmaybe-uninitialized]
  206 |     quintupletsInGPU.rzChiSquared[quintupletIndex] = rzChiSquared;
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
Quintuplet.h: In function 'void SDL::createQuintupletsInGPUv2::operator()(const TAcc&, SDL::modules, SDL::miniDoublets, SDL::segments, SDL::triplets, SDL::quintuplets, SDL::objectRanges, uint16_t) const [with TAcc = alpaka::AccCpuSerial<std::integral_constant<long unsigned int, 3>, long unsigned int>]':
Quintuplet.h:3051:103: note: 'rzChiSquared' was declared here
 3051 |             float innerRadius, outerRadius, bridgeRadius, regressionG, regressionF, regressionRadius, rzChiSquared,
      |   

But this is really nice. It's great so see such a clean compilation log.

@slava77
Copy link
Contributor Author

slava77 commented Apr 5, 2024

When compiling for cpu I get one last warning. (It's pretty easy to fix)

I'm curious why I don't get this warning

@slava77
Copy link
Contributor Author

slava77 commented Apr 5, 2024

When compiling for cpu I get one last warning. (It's pretty easy to fix)

I'm curious why I don't get this warning

are you compiling in a special setup without DUSE_RZCHI2 ?

…osed to initialize; fix maybe-uninitialized for builds without DNN and without USE_RZCHI2
@slava77
Copy link
Contributor Author

slava77 commented Apr 5, 2024

When compiling for cpu I get one last warning. (It's pretty easy to fix)

please check after 8f67f82

@slava77
Copy link
Contributor Author

slava77 commented Apr 5, 2024

/run standalone
/run cmssw

Copy link

github-actions bot commented Apr 5, 2024

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Copy link

github-actions bot commented Apr 6, 2024

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

@ariostas
Copy link
Member

ariostas commented Apr 6, 2024

are you compiling in a special setup without DUSE_RZCHI2 ?

Oh yeah, I was testing it directly running make and I forgot to include some of the extra flags. There are no warnings now regardless of the flags.

Now that I think about it, I also need to fix LSTCore so that it uses all these extra flags.

Thank you, @slava77!

@ariostas ariostas merged commit fc97a6b into SegmentLinking:master Apr 6, 2024
3 checks passed
@ariostas
Copy link
Member

ariostas commented Apr 9, 2024

I just noticed that the ROCm compilation in CMSSW seems to use a couple of extra warning flags, but I guess we can worry about that later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect and Ambiguous Uses of Cuda Memset
4 participants